Can we just omit this here, and rely on the if len(errs) > 0 { block below? (Which would require putting the s.VMFolder = folders.VmFolder line in an else block, of course.)

under some circumstances (tho very rare), s.Datacenter.Folders(op) returns no error (err == nil) but the folders return is nil. (for example, some reference handling race condition in vSphere or sth)
this is included in the comment a little above it

Also errs include errors from other stuff, checking for a specific one requires iterating and it doesn't feel very neat

I don't think I'm suggesting anything that'd require iterating.

Right now, you have:

	if s.Datacenter != nil {
		folders, err := s.Datacenter.Folders(op)
		if err != nil {
			errs = append(errs, fmt.Sprintf("Failure finding folders (%s): %s", s.DatacenterPath, err.Error()))
		} else {
			op.Debugf("Cached folders: %s", s.DatacenterPath)
		}
		// There could be cases where no error from Datacenter.Folders, but nil folder is returned. In this case we should bail out.
		// It's also possible that there's an error, but a valid vm folder is returned
		if folders == nil {
			errs = append(errs, fmt.Sprintf("Nil folder returned when finding folders (%s)", s.DatacenterPath))
			return nil, errors.New(strings.Join(errs, "\n"))
		}

		s.VMFolder = folders.VmFolder
	}

	if len(errs) > 0 {
		op.Debugf("Error count populating vSphere cache: (%d)", len(errs))
		return nil, errors.New(strings.Join(errs, "\n"))
	}
	op.Debug("vSphere resource cache populated...")
	return s, nil

I think you could just remove the return you added without changing the behavior:

	if s.Datacenter != nil {
		folders, err := s.Datacenter.Folders(op)
		if err != nil {
			errs = append(errs, fmt.Sprintf("Failure finding folders (%s): %s", s.DatacenterPath, err.Error()))
		} else {
			op.Debugf("Cached folders: %s", s.DatacenterPath)
		}
		// There could be cases where no error from Datacenter.Folders, but nil folder is returned. In this case we should bail out.
		// It's also possible that there's an error, but a valid vm folder is returned
		if folders == nil {
			errs = append(errs, fmt.Sprintf("Nil folder returned when finding folders (%s)", s.DatacenterPath))
-			return nil, errors.New(strings.Join(errs, "\n"))
-		}
-		
+		} else {
			s.VMFolder = folders.VmFolder
+		}
	}

	if len(errs) > 0 {
		op.Debugf("Error count populating vSphere cache: (%d)", len(errs))
		return nil, errors.New(strings.Join(errs, "\n"))
	}
	op.Debug("vSphere resource cache populated...")
	return s, nil

The behavior should still be the same; since you're appending to errs in the folder == nil block, len(errs) will be > 0, and you'll return the same thing at the end:

	if s.Datacenter != nil {
		folders, err := s.Datacenter.Folders(op)
		if err != nil {
			errs = append(errs, fmt.Sprintf("Failure finding folders (%s): %s", s.DatacenterPath, err.Error()))
		} else {
			op.Debugf("Cached folders: %s", s.DatacenterPath)
		}
		// There could be cases where no error from Datacenter.Folders, but nil folder is returned. In this case we should bail out.
		// It's also possible that there's an error, but a valid vm folder is returned
		if folders == nil {
			errs = append(errs, fmt.Sprintf("Nil folder returned when finding folders (%s)", s.DatacenterPath))
		} else {
			s.VMFolder = folders.VmFolder
		}
	}

	if len(errs) > 0 {
		op.Debugf("Error count populating vSphere cache: (%d)", len(errs))
		return nil, errors.New(strings.Join(errs, "\n"))
	}
	op.Debug("vSphere resource cache populated...")
	return s, nil

This essentially just preserves the pattern the code was already using: accumulate a collection of errors and then return all of them. This is important because if someone adds another check after the if s.Datacenter != nil block, you'd want to include any errors from that too.

I think what I wanted was to make the session.Populate early abort when folders == nil, without doing s.VMFolder = folders.VmFolder step that dumps core when folders == nil.

Preserving the error accumulation pattern makes perfect sense. To make it cleaner, I think I could do sth like this:

		if folders == nil {
			errs = append(errs, fmt.Sprintf("Nil folder returned when finding folders (%s)", s.DatacenterPath))
		} else {
                      s.VMFolder = folders.VmFolder 
                }

And then it's the validator's job to check session.VMFolder and quit if it's empty.

I think this may cause issues when doing a vic-machine ls with multiple datacenters.

I suggest delaying any further work on this until @matthewavery has delivered the inventory folder support (#773) and then revisting it.

makes sense. I will then leave the PR as it is for now, and move the ticket back to To-Do. Thanks!